fix: close weighted RRF review gaps#588
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3de21835-81f6-40f7-b776-b3434db7cc0a) |
|
@codex review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
📝 WalkthroughWalkthrough
ChangesWarm RRF search
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_hybrid_search.py`:
- Around line 287-318: Add a dedicated test beside
test_warm_fts_fallback_preserves_fts_rank_when_alpha_is_one covering the
symmetric warm_rrf branch where semantic results exist but the FTS leg is empty.
Set BRAINLAYER_RRF_ALPHA to 0.0, insert close and far vector matches with query
text guaranteed to have no lexical matches, build the binary index, capture
_mmr_rerank_scored_results, and assert pure-vector ordering and positive
descending scores.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4127359b-fc66-4a32-9262-08a17c1c69c5
📒 Files selected for processing (5)
src/brainlayer/engine.pysrc/brainlayer/mcp/search_handler.pysrc/brainlayer/search_repo.pytests/test_hybrid_search.pytests/test_search_handler.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: CodeRabbit / Review
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior
Enforce one-write-at-a-time concurrency constraint; reads are safe but brain_digest is write-heavy and must not run in parallel with other MCP work
Run pytest before claiming behavior changed safely; current test suite has 929 tests
Files:
tests/test_search_handler.pysrc/brainlayer/engine.pysrc/brainlayer/mcp/search_handler.pytests/test_hybrid_search.pysrc/brainlayer/search_repo.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not run
tests/test_vector_store.pyortests/test_engine.pyduring worker pre-push checks; run them only as deliberate production-database checks.
Files:
tests/test_search_handler.pytests/test_hybrid_search.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Usepaths.py:get_db_path()for database path resolution in all scripts and CLI code.
Retry onSQLITE_BUSY; each worker must use its own database connection.
Files:
src/brainlayer/engine.pysrc/brainlayer/mcp/search_handler.pysrc/brainlayer/search_repo.py
src/brainlayer/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/brainlayer/**/*.py: Preserveai_code,stack_trace, anduser_messageverbatim; skip noise, summarize build logs, and retain only structure for directory listings.
Use AST-aware tree-sitter chunking, never split stack traces, and mask large tool output.
Use Groq as the primary enrichment backend, Gemini as fallback, and Ollama as the offline last resort; honorBRAINLAYER_ENRICH_BACKENDandBRAINLAYER_ENRICH_RATE.
Default searches must exclude lifecycle-managed chunks;include_archived=Truemay show history.
brain_supersedemust apply a safety gate to personal data, andbrain_archivemust perform a timestamped soft delete.
brain_storemust support an atomicsupersedesstore-and-replace operation.
Production backup logs must identify real runs withbackup_log_provenance=real; tests must useBRAINLAYER_BACKUP_LOG_PATHand provenancepytestso they cannot modify the production heartbeat log.
Before bulk database operations, stop enrichment workers, checkpoint WAL before and after withPRAGMA wal_checkpoint(FULL), drop and recreate FTS triggers around large deletes, batch deletes in 5–10K chunks, and checkpoint every three batches.
Files:
src/brainlayer/engine.pysrc/brainlayer/mcp/search_handler.pysrc/brainlayer/search_repo.py
src/brainlayer/mcp/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/brainlayer/mcp/**/*.py: Use the fixed-size read-only WALVectorStorepool; respectBRAINLAYER_READ_POOL_SIZE,BRAINLAYER_READ_BUSY_TIMEOUT_MS, and the approximately 768 MB pool/cache startup limit.
Add new MCP tool implementations in themcp/directory and expose them through thebrainlayer-mcpentrypoint.
Files:
src/brainlayer/mcp/search_handler.py
🔇 Additional comments (7)
src/brainlayer/engine.py (1)
180-185: LGTM!Also applies to: 209-217, 246-254, 287-297, 312-323
src/brainlayer/mcp/search_handler.py (1)
2456-2459: LGTM!Also applies to: 2507-2510
src/brainlayer/search_repo.py (2)
338-371: LGTM! Recency-decay is cleanly isolated from the recency-rerank candidate fallback, and the cache key correctly reflects the effective decay state.Also applies to: 1806-1806, 1842-1842, 2448-2448
2326-2333: Leg-aware fusion alpha correctly preserves the surviving retrieval leg.The branch order is correct: forcing
alpha=0.0when the semantic leg is empty andalpha=1.0when the FTS leg is empty prevents a configuredBRAINLAYER_RRF_ALPHAfrom zeroing out the only leg that actually returned candidates. Confirmed against_weighted_rrf_score's formula that this yields the intended single-leg ranking.Also applies to: 2345-2345
tests/test_hybrid_search.py (2)
287-318: LGTM! Correctly validates the semantic-empty fallback branch of the new fusion-alpha logic.
444-461: LGTM! Good regression test isolating the recency-decay env flag from the recent-candidate SQL fallback.tests/test_search_handler.py (1)
446-449: LGTM!Also applies to: 841-844, 863-866
| def test_warm_fts_fallback_preserves_fts_rank_when_alpha_is_one(self, store, monkeypatch): | ||
| monkeypatch.setenv("BRAINLAYER_RRF_ALPHA", "1.0") | ||
| _insert_chunk( | ||
| store, | ||
| chunk_id="exact-fts", | ||
| content="alpha fallback", | ||
| embedding=_embed("distant exact"), | ||
| ) | ||
| _insert_chunk( | ||
| store, | ||
| chunk_id="verbose-fts", | ||
| content="alpha fallback with several unrelated padding words", | ||
| embedding=_embed("distant verbose"), | ||
| ) | ||
| captured: list[tuple] = [] | ||
|
|
||
| def capture_ranked(scored, *, n_results): | ||
| captured.extend(scored) | ||
| return scored | ||
|
|
||
| monkeypatch.setattr(store, "_mmr_rerank_scored_results", capture_ranked) | ||
|
|
||
| store.hybrid_search( | ||
| query_embedding=None, | ||
| query_text="alpha fallback", | ||
| n_results=2, | ||
| warm_rrf=True, | ||
| ) | ||
|
|
||
| assert [item[1] for item in captured] == ["exact-fts", "verbose-fts"] | ||
| assert captured[0][0] > captured[1][0] > 0.0 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing mirror-branch coverage for the fts-empty fusion-alpha case.
Only the "semantic leg empty → alpha=0.0" branch (this test) is covered. The symmetric branch — semantic leg populated but FTS leg empty (warm_rrf and not fts_ranks: fusion_alpha = 1.0 in search_repo.py) — has no dedicated test asserting it forces pure-vector ranking under a low BRAINLAYER_RRF_ALPHA.
✅ Suggested additional test
def test_warm_semantic_fallback_preserves_vector_rank_when_alpha_is_zero(self, store, monkeypatch):
monkeypatch.setenv("BRAINLAYER_RRF_ALPHA", "0.0")
query_embedding = _embed("semantic fallback query")
_insert_chunk(
store,
chunk_id="close-vec",
content="tokens with zero overlap versus the query text",
embedding=query_embedding,
)
_insert_chunk(
store,
chunk_id="far-vec",
content="tokens with zero overlap versus the query text too",
embedding=[v + 0.01 for v in query_embedding],
)
store.build_binary_index()
captured: list[tuple] = []
def capture_ranked(scored, *, n_results):
captured.extend(scored)
return scored
monkeypatch.setattr(store, "_mmr_rerank_scored_results", capture_ranked)
store.hybrid_search(
query_embedding=query_embedding,
query_text="xyzzy plugh unmatched lexical tokens",
n_results=2,
warm_rrf=True,
)
assert [item[1] for item in captured] == ["close-vec", "far-vec"]
assert captured[0][0] > captured[1][0] > 0.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_hybrid_search.py` around lines 287 - 318, Add a dedicated test
beside test_warm_fts_fallback_preserves_fts_rank_when_alpha_is_one covering the
symmetric warm_rrf branch where semantic results exist but the FTS leg is empty.
Set BRAINLAYER_RRF_ALPHA to 0.0, insert close and far vector matches with query
text guaranteed to have no lexical matches, build the binary index, capture
_mmr_rerank_scored_results, and assert pure-vector ordering and positive
descending scores.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
brain_searchthink/recall paths through warm two-leg RRFContext
PRs #584 and #587 were merged by the lead before their review follow-up commits reached the merged heads. This recovery PR carries the omitted #584 fix and resolves both unresolved #587 inline findings.
Test plan
Note
Medium Risk
Changes hybrid search ranking and MCP think/recall retrieval paths; behavior shifts are intentional but could affect result ordering in edge cases (single-leg search, recency env).
Overview
Fixes follow-up gaps in weighted RRF and recency scoring after earlier merges.
Recency behavior is split so
BRAINLAYER_SCORE_RECENCY_DECAY(and explicitrecency_rerank) only drive post-fusion score decay via a new internalrecency_decayflag. That env opt-in no longer implies pulling in unrelated recent chunks when lexical/vector legs miss.Warm two-leg RRF now adjusts fusion
alphawhen only one leg has candidates (FTS-only or vector-only), so a degraded search still ranks by the surviving leg instead of wiping its contribution when config weight would zero it out.MCP smart routes pass
warm_rrf=Truefromthink/recallinengine.pythroughsearch_handler, aligning interactive memory retrieval with warm vector+FTS fusion (no legacy trigram leg on those paths).Tests cover FTS-only warm fallback ordering, env recency decay without spurious recent hits, and handler wiring for
warm_rrf.Reviewed by Cursor Bugbot for commit c6207d2. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix weighted RRF review gaps by propagating
warm_rrfthrough search pathswarm_rrfboolean parameter toengine.thinkandengine.recallin engine.py, forwarded intohybrid_searchcalls.warm_rrf=Truein the MCP search handler for both_thinkand_recallcoroutines in search_handler.py.BRAINLAYER_SCORE_RECENCY_DECAYwas incorrectly togglingrecency_rerankinstead of a distinctrecency_decayvariable; also extends_hybrid_cache_keyto includerecency_decayfor proper cache segregation.BRAINLAYER_SCORE_RECENCY_DECAYno longer influencesrecency_rerankdirectly, which may change which recency behaviors are active at runtime for deployments using that flag.Macroscope summarized c6207d2.
Summary by CodeRabbit